-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn users when there are pending operations but proceed with deployment #9293
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very welcomed change! ❤️
@@ -550,11 +550,6 @@ func TestPreviewWithPendingOperations(t *testing.T) { | |||
// A preview should succeed despite the pending operations. | |||
_, res := op.Run(project, target, options, true, nil, nil) | |||
assert.Nil(t, res) | |||
|
|||
// But an update should fail. | |||
_, res = op.Run(project, target, options, false, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test we get the expected warning here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see this is now just for preview and we have tests elsewhere for updates.
@@ -605,11 +600,6 @@ func TestRefreshWithPendingOperations(t *testing.T) { | |||
options := UpdateOptions{Host: deploytest.NewPluginHost(nil, nil, program, loaders...)} | |||
project, target := p.GetProject(), p.GetTarget(t, old) | |||
|
|||
// Without refreshing, an update should fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
// Print a warning for users that there are pending operations. | ||
// Explain that these operations can be cleared using pulumi refresh (except for CREATE operations) | ||
// since these require user intevention: | ||
ex.printPendingOperationsWarning() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to what we did with refresh I think we might want to do something to ensure that after doing the update our snapshot still tracks that there were pending operations that were not resovled.
Thanks @Frassle, I will revisit the points and try add some tests 😄 |
@Frassle Added tests
Can you please have a look? 🙏 |
@@ -550,11 +550,6 @@ func TestPreviewWithPendingOperations(t *testing.T) { | |||
// A preview should succeed despite the pending operations. | |||
_, res := op.Run(project, target, options, true, nil, nil) | |||
assert.Nil(t, res) | |||
|
|||
// But an update should fail. | |||
_, res = op.Run(project, target, options, false, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see this is now just for preview and we have tests elsewhere for updates.
} | ||
|
||
_, res := op.Run(project, target, options, false, nil, validate) | ||
assert.Nil(t, res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the refresh test above can we assert that the pending operations are still present (and we should test both PendingUpdates and PendingCreates are retained)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am writing the asserts now and it seems that only pending creates are preserved on an update (just like refresh) is this by design? 🤔
// THIS FAILS: expected 2, actual 1
assert.Equal(t, 2, len(new.PendingOperations))
assert.Equal(t, resource.OperationTypeUpdating, new.PendingOperations[0].Type)
assert.Equal(t, urnA, new.PendingOperations[0].Resource.URN)
assert.Equal(t, resource.OperationTypeCreating, new.PendingOperations[1].Type)
assert.Equal(t, urnB, new.PendingOperations[1].Resource.URN)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that might be ok, I don't think it's ideal but probably good enough for this work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, added asserts for retaining pending create operations, this way the behaviour is documented until we revisit a better solution
Description
Fixes #4265
This PR changes the deployment execution such that it no longer errors out when there are pending operations. Instead, it only warns the users and explains that they can use
pulumi refresh
to clear the operations and they will no longer see the warning (unless there are pendingcreate
operations)Checklist